Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change the default port for OTLP HTTP #373

Merged
merged 3 commits into from
Aug 6, 2021
Merged

Change the default port for OTLP HTTP #373

merged 3 commits into from
Aug 6, 2021
+42 −11

Conversation

joaopgrassi
Copy link
Member

@joaopgrassi joaopgrassi commented Aug 4, 2021 •
edited
Loading

From the issue:

  • make sure we create a new -legacy port, for 55681
  • change the default port from 55681 to 4318
  • schedule a ticket to remove the legacy port in two releases

resolves #371

@joaopgrassi
Change the default port for OLTP http
f28a79c 
And marking the existing as legacy.
@joaopgrassi
Copy link
Member Author

Hey all,
I tried running the tests locally before opening the PR, but I ran into this issue:

o install sigs.k8s.io/kustomize/kustomize/[email protected]: sigs.k8s.io/kustomize/kustomize/[email protected]
	The go.mod file for the module providing named packages contains one or
	more exclude directives. It must not contain directives that would cause
	it to be interpreted differently than if it were the main module.
make: *** [Makefile:155: kustomize] Error 1

My Go installation is: go version go1.16.6 linux/amd64 I tried running make test with minikube and also by using kubebuilder (installed using the install-kubebuilder.sh file)

I'm rather new to go, so sorry if this is a stupid question 🙈

jpkrohling
jpkrohling previously approved these changes Aug 5, 2021
View reviewed changes
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jpkrohling
Copy link
Member

I don't see the same problem as you are seeing. I do get a test failure, though:

$ make test
/home/jpkroehling/go/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
go fmt ./...
go vet ./...
/home/jpkroehling/go/bin/controller-gen "crd:trivialVersions=true,preserveUnknownFields=false,generateEmbeddedObjectMeta=true" rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=config/crd/bases
cd config/manager && /home/jpkroehling/bin/kustomize edit set image controller=quay.io/opentelemetry/opentelemetry-operator:v"0.31.0"
/usr/bin/operator-sdk generate kustomize manifests -q
WARN[0000] Skipping definitions parsing for API {}: Go type not found 
/home/jpkroehling/bin/kustomize build config/manifests | /usr/bin/operator-sdk generate bundle -q --overwrite --manifests --version "0.31.0"  
/usr/bin/operator-sdk bundle validate ./bundle
INFO[0000] Found annotations file                        bundle-dir=bundle container-tool=docker
INFO[0000] Could not find optional dependencies file     bundle-dir=bundle container-tool=docker
INFO[0000] All validation tests have completed successfully 
# on make bundle config/manager/kustomization.yaml includes changes, which should be ignored for the below check
go test -race ./...
?   	github.com/open-telemetry/opentelemetry-operator	[no test files]
?   	github.com/open-telemetry/opentelemetry-operator/api/v1alpha1	[no test files]
ok  	github.com/open-telemetry/opentelemetry-operator/controllers	6.867s
ok  	github.com/open-telemetry/opentelemetry-operator/internal/config	0.179s
ok  	github.com/open-telemetry/opentelemetry-operator/internal/podinjector	7.221s
ok  	github.com/open-telemetry/opentelemetry-operator/internal/version	0.028s
ok  	github.com/open-telemetry/opentelemetry-operator/pkg/autodetect	0.087s
ok  	github.com/open-telemetry/opentelemetry-operator/pkg/collector	0.105s
ok  	github.com/open-telemetry/opentelemetry-operator/pkg/collector/adapters	0.054s
--- FAIL: TestOTLPExposeDefaultPorts (0.00s)
    receiver_otlp_test.go:97: 
        	Error Trace:	receiver_otlp_test.go:97
        	Error:      	"[{otlp-grpc  %!s(*string=<nil>) %!s(int32=4317) {%!s(intstr.Type=0) %!s(int32=4317) } %!s(int32=0)} {otlp-http  %!s(*string=<nil>) %!s(int32=4318) {%!s(intstr.Type=0) %!s(int32=4318) } %!s(int32=0)} {otlp-http-legacy  %!s(*string=<nil>) %!s(int32=55681) {%!s(intstr.Type=0) %!s(int32=4318) } %!s(int32=0)}]" should have 1 item(s), but has 3
        	Test:       	TestOTLPExposeDefaultPorts
    receiver_otlp_test.go:103: 
        	Error Trace:	receiver_otlp_test.go:103
        	Error:      	Not equal: 
        	            	expected: 0
        	            	actual  : 4318
        	Test:       	TestOTLPExposeDefaultPorts
FAIL
FAIL	github.com/open-telemetry/opentelemetry-operator/pkg/collector/parser	0.045s
ok  	github.com/open-telemetry/opentelemetry-operator/pkg/collector/reconcile	7.928s
ok  	github.com/open-telemetry/opentelemetry-operator/pkg/collector/upgrade	6.653s
?   	github.com/open-telemetry/opentelemetry-operator/pkg/naming	[no test files]
?   	github.com/open-telemetry/opentelemetry-operator/pkg/platform	[no test files]
ok  	github.com/open-telemetry/opentelemetry-operator/pkg/sidecar	0.085s
FAIL
make: *** [Makefile:60: test] Error 1

@jpkrohling
Copy link
Member

I found the issue you are facing: you can reproduce it by running make kustomize. The workaround is to manually install kustomize, instead of letting the Makefile install it for you.

@jpkrohling jpkrohling mentioned this pull request Aug 5, 2021
Closed
@joaopgrassi
Copy link
Member Author

joaopgrassi commented Aug 5, 2021 •
edited
Loading

I found the issue you are facing: you can reproduce it by running make kustomize. The workaround is to manually install kustomize, instead of letting the Makefile install it for you.

Nice, thanks @jpkrohling ! I'll fix the tests later today and publish the PR.

@mergify mergify bot dismissed jpkrohling’s stale review August 5, 2021 19:02

Pull request has been modified.

@joaopgrassi joaopgrassi requested a review from jpkrohling August 5, 2021 19:12
@joaopgrassi joaopgrassi marked this pull request as ready for review August 5, 2021 19:14
@joaopgrassi joaopgrassi requested review from a team August 5, 2021 19:14
@jpkrohling jpkrohling enabled auto-merge (squash) August 6, 2021 07:49
@jpkrohling jpkrohling merged commit 3eae0e5 into open-telemetry:main Aug 6, 2021
@joaopgrassi joaopgrassi changed the title Change the default port for OLTP HTTP Change the default port for OTLP HTTP Aug 6, 2021
@jpkrohling jpkrohling added this to the v0.33.0 milestone Aug 19, 2021
shree007 pushed a commit to shree007/opentelemetry-operator that referenced this pull request Dec 12, 2021
@joaopgrassi
Change the default port for OLTP HTTP (open-telemetry#373)
31b949a 
* Change the default port for OLTP http
And marking the existing as legacy.

* Fix tests
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
@joaopgrassi
Change the default port for OLTP HTTP (open-telemetry#373)
edd6ce3 
* Change the default port for OLTP http
And marking the existing as legacy.

* Fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpkrohling jpkrohling jpkrohling approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.33.0
Development

Successfully merging this pull request may close these issues.

New default port for OTLP HTTP
2 participants
@joaopgrassi @jpkrohling